Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Background image: update controls defaults and layout #62000

Merged
merged 4 commits into from
May 28, 2024

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented May 27, 2024

What?

Maybe resolves #61928

This PR:

  • harmonizes headings between block supports and global styles
  • updates copy including changing the label "Fixed" to "Tile"
  • uses horizontal layout for image size and repeat
  • fixes bug where width input is cleared and the size defaults to "cover"
  • tidies up the visually hidden context labels

Why?

Trying to bring some consistency to the controls.

Testing Instructions

... To come

Screenshots or screencast

Screenshot 2024-05-27 at 3 28 10 pm

…-wide background image in the editor by settings the value to `background-image: none`
…the label "Fixed" to "Tile", using horizontal layout for image size and repeat, fixing bug where width input is cleared and the size defaults to "cover"
@ramonjd ramonjd added [Type] Enhancement A suggestion for improvement. [Status] In Progress Tracking issues with work in progress Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels May 27, 2024
@ramonjd ramonjd requested a review from jasmussen May 27, 2024 05:28
@ramonjd ramonjd self-assigned this May 27, 2024
@ramonjd ramonjd requested a review from ellatrix as a code owner May 27, 2024 05:28
Copy link

github-actions bot commented May 27, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ramonjd <[email protected]>
Co-authored-by: jasmussen <[email protected]>
Co-authored-by: andrewserong <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

…defaults' into update/background-image-control-defaults
…ge title or image filename.

Where there is no image URL, then show no background image selected
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fast work, big improvement. Bug is fixed, and this is coherent with the Group option as well:

group block background image in inspector

As I argue in this comment, I think we should also still change the default from Tile to Cover. The short summary of why is: until we have background-size and background-position tools to enable 2x resolution, and top/centered as opposed to top/left tiling position, "Tile" as a default is not going to be a good or expected experience.

That's not a blocker, so we can discuss this separately if need be.

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is testing well for me, too, and adds a nice level of polish. (Great idea @jasmussen to use the name Tile, too, it fits much better than Fixed, and allows for a true fixed option further down the track).

I think we should also still change the default from Tile to Cover ... That's not a blocker, so we can discuss this separately if need be.

On this one, I still feel like Cover for site wide backgrounds as a default risks leading users astray, so IMO a separate PR for switching the defaults would be a good way to go if we're planning to change it, as that way we've got the change clearly documented in its own PR.

This one LGTM! 🚀

@ramonjd
Copy link
Member Author

ramonjd commented May 28, 2024

I appreciate the test and feedback, folks!

I still feel like Cover for site wide backgrounds as a default risks leading users astray

I'm also for leaving it set to "auto" by default, but for different reasons.

For theme.json, I think we should be wary of inserting any default values that a user might not expect. My view is that theme.json should primarily represent positive intent, that is, "turn stuff on", rather than override opinionated CMS rules.

@ramonjd ramonjd merged commit b3cfec4 into trunk May 28, 2024
63 of 64 checks passed
@ramonjd ramonjd deleted the update/background-image-control-defaults branch May 28, 2024 00:04
@github-actions github-actions bot added this to the Gutenberg 18.5 milestone May 28, 2024
@ramonjd
Copy link
Member Author

ramonjd commented May 28, 2024

For theme.json, I think we should be wary of inserting any default values that a user might not expect.

Oh, and the backwards compat "fun" there'd be if folks ever wanted to change/roll back the default. 🤣

@jasmussen
Copy link
Contributor

Thanks for moving fast on this one, also carefully. I did mention that I think Cover may be a better default experience, but as it turns out, I was misremembering aspects of this implementation, which in more in-depth testing reveals that, well, there probably really isn't any good default here, unless we add another bit of bundled default code, which I'll get back to.

My instinct was for Cover to solve these issues:

  • It solves the low resolution issue without adding a background-size tool, and without precluding such a tool landing in the future.
  • It solves the problem of defaulting to top/left alignment, where arguably most sites are top/center aligned, without adding a background-position tool.
  • It solves the problem of images being huge on mobile, small on desktop, because it doesn't scale across.

Turns out I was thinking of the behavior of Contain, and even in that case, it only works with Contain + repeat, and then only for landscape images. Here's a case where that works well:

contain-desktop

contain-mobile

But even that falls apart if the image is portrait oriented. At that point it's still top/left aligned, but tiles horizontally.

For reference, here's the current default, Tile, which as noted has its own problems:
tile

tile-mobile

I'll dive back into #62046 with another suggestion, yet another new default!

@burnuser
Copy link

burnuser commented May 29, 2024

"Cover" would be the best default, when used like in good old Customizer as cover the screen (not the whole webpage!) in combination with "background-attachment: fixed"
(In fact I've never seen a website with a scrolling background image in real live.)

So, if there is no comparable solution to Customizer default, this feature is - in my opinion - not ready for WP 6.6

@ramonjd
Copy link
Member Author

ramonjd commented May 29, 2024

Thanks for the input @burnuser 🙇🏻

"Cover" would be the best default, when used like in good old Customizer as cover the screen (not the whole webpage!) in combination with "background-attachment: fixed"

Background attachment controls are in the works:

But it won't be part of 6.6 unfortunately. I'm working on this stuff as fast as I can 😄

As for the background-size: cover as the default, it's already the default for blocks (Group Block), but not for the site. It can be set in theme.json by specifying "backgroundSize": "cover", or via the Site Editor tools however.

In my opinion, we should be cautious about assuming defaults in theme.json in the absence of any values.

In the case of background size, the CSS default is "auto". For WordPress to assume "cover" universally might look fine for some images, but it won't for all. Furthermore, if, for any reason, that default needs to change in the future, then WordPress would have to continue to honor "cover" due to backwards compatibility forever.

So, if there is no comparable solution to Customizer default, this feature is - in my opinion - not ready for WP 6.6

There's a discussion going on how best background images in block themes can play with the current Customizer tool.

The current state is that background images set via the Customizer will take precedence over those set in theme.json due to CSS specificity, so you can continue to use it without any interference from this new feature.

@ramonjd ramonjd removed the [Status] In Progress Tracking issues with work in progress label May 29, 2024
carstingaxion pushed a commit to carstingaxion/gutenberg that referenced this pull request Jun 4, 2024
* Background image: add functionality to remove a theme's default, site-wide background image in the editor by settings the value to `background-image: none`

* Background image: harmonize headings, update copy including changing the label "Fixed" to "Tile", using horizontal layout for image size and repeat, fixing bug where width input is cleared and the size defaults to "cover"

* Make sure the visually hidden labels are correct - use either the image title or image filename.
Where there is no image URL, then show no background image selected

Co-authored-by: ramonjd <[email protected]>
Co-authored-by: jasmussen <[email protected]>
Co-authored-by: andrewserong <[email protected]>
patil-vipul pushed a commit to patil-vipul/gutenberg that referenced this pull request Jun 17, 2024
* Background image: add functionality to remove a theme's default, site-wide background image in the editor by settings the value to `background-image: none`

* Background image: harmonize headings, update copy including changing the label "Fixed" to "Tile", using horizontal layout for image size and repeat, fixing bug where width input is cleared and the size defaults to "cover"

* Make sure the visually hidden labels are correct - use either the image title or image filename.
Where there is no image URL, then show no background image selected

Co-authored-by: ramonjd <[email protected]>
Co-authored-by: jasmussen <[email protected]>
Co-authored-by: andrewserong <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Background image tool: Improvements, bugfixes, find alternative to "fixed"
4 participants